Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DAOS-16936 dtx: rank range based DTX hints for coll_punch #15979

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

DAOS-16936 dtx: rank range based DTX hints for coll_punch #15979

wants to merge 1 commit into from
+193 −49

Conversation

Nasf-Fan
Copy link
Contributor

When handle collective punch RPC, the leader will generate hints array for related DTX RPC (abort or commit). Each involved engine will have one element in such array. To simplify RPC logic, such hints array is sparse and indexed with engine's rank#. Originally, we did not properly handled the case of incontinuous rank# in the pool map, as to when update some hints element with large rank#, the write maybe out of boundary and crash the others' space and cause kinds of DRAM corruption.

Similar situation can happen during handle collective DTX resync and cleanup.

This patch fixes such issue via building such sparse hints array based on related engines' rank# range instead of the ranks count in the pool map. Use relative ranks diff (real rank# - base rank#) as the index. That will avoid out of boundary access.

Steps for the author:

  • Commit message follows the guidelines.
  • Appropriate Features or Test-tag pragmas were used.
  • At least two positive code reviews including at least one code owner from each category referenced in the PR.
  • Testing is complete. If necessary, forced-landing label added and a reason added in a comment.

After all prior steps are complete:

  • Gatekeeper requested (daos-gatekeeper added as a reviewer).

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 26, 2025 •
edited
Loading

Ticket title is 'Aurora: assertion with collective punch running mdtest with 2048 ECBS'
Status is 'In Review'
Labels: 'ALCF,alcf_track,post_acceptance_issues,scrubbed_2.8'
https://daosio.atlassian.net/browse/DAOS-16936

@daosbuild1
Copy link
Collaborator

Test stage NLT on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15979/1/testReport/

@Nasf-Fan Nasf-Fan force-pushed the Nasf-Fan/DAOS-16936_1 branch from d30dc42 to 6a4b687 Compare February 26, 2025 06:53
@daosbuild1
Copy link
Collaborator

Test stage NLT on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15979/2/testReport/

@Nasf-Fan Nasf-Fan force-pushed the Nasf-Fan/DAOS-16936_1 branch from 6a4b687 to 15e44d6 Compare February 26, 2025 15:07
mchaarawi
mchaarawi reviewed Feb 26, 2025
View reviewed changes
Copy link
Contributor

@mchaarawi mchaarawi left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the PR should re-enable collective punch as default if the fix was for it.
otherwise no test will be running with collective punch in CI anyway.

@Nasf-Fan
Copy link
Contributor Author

i think the PR should re-enable collective punch as default if the fix was for it. otherwise no test will be running with collective punch in CI anyway.

Yes, collective punch has already been enabled on master, but disabled on release/2.6 by default. This patch is for master, so no need to change that.

@Nasf-Fan
Copy link
Contributor Author

https://github.com/daos-stack/daos/actions/runs/13547004393/job/37861024957?pr=15979
diff --git a/src/dtx/dtx_internal.h b/src/dtx/dtx_internal.h
index 6a7442c69..0250db7b9 100644
--- a/src/dtx/dtx_internal.h
+++ b/src/dtx/dtx_internal.h
@@ -77,16 +77,11 @@ CRT_RPC_DECLARE(dtx, DAOS_ISEQ_DTX, DAOS_OSEQ_DTX);

  • dci_hints is sparse array, one per engine, sorted against the rank ID.
  • It can hold more than 19K engines inline RPC body.
    */
    -#define DAOS_ISEQ_COLL_DTX \
  • ((uuid_t) (dci_po_uuid) CRT_VAR) \
  • ((uuid_t) (dci_co_uuid) CRT_VAR) \
  • ((struct dtx_id) (dci_xid) CRT_VAR) \
  • ((uint32_t) (dci_version) CRT_VAR) \
  • ((uint32_t) (dci_min_rank) CRT_VAR) \
  • ((uint32_t) (dci_max_rank) CRT_VAR) \
  • ((uint32_t) (dci_padding) CRT_VAR) \
  • ((uint64_t) (dci_epoch) CRT_VAR) \
  • ((uint8_t) (dci_hints) CRT_ARRAY)
    +#define DAOS_ISEQ_COLL_DTX \
  • ((uuid_t)(dci_po_uuid)CRT_VAR)((uuid_t)(dci_co_uuid)CRT_VAR)((struct dtx_id)( \
  •   dci_xid)CRT_VAR)((uint32_t)(dci_version)CRT_VAR)((uint32_t)(dci_min_rank)CRT_VAR)(     \

  •   (uint32_t)(dci_max_rank)CRT_VAR)((uint32_t)(dci_padding)CRT_VAR)(                      \

  •   (uint64_t)(dci_epoch)CRT_VAR)((uint8_t)(dci_hints)CRT_ARRAY)

@daltonbohning ,
Honestly, I do not think that the suggested clang-format is friendly for both engineers and other readers. I am wondering why we have to follow such format? or is there any rule can be adjusted? Thanks!

@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15979/3/execution/node/1510/log

@Nasf-Fan
Copy link
Contributor Author

Test stage Functional Hardware Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15979/3/execution/node/1510/log

Failed for DAOS-17094.

@Nasf-Fan Nasf-Fan marked this pull request as ready for review February 27, 2025 13:45
@Nasf-Fan Nasf-Fan requested review from a team as code owners February 27, 2025 13:45
@daltonbohning
Copy link
Contributor

@daltonbohning , Honestly, I do not think that the suggested clang-format is friendly for both engineers and other readers. I am wondering why we have to follow such format? or is there any rule can be adjusted? Thanks!

The hook will format for you
https://github.com/daos-stack/daos/blob/master/utils/githooks/README.md

@jolivier23
Copy link
Contributor

https://github.com/daos-stack/daos/actions/runs/13547004393/job/37861024957?pr=15979 diff --git a/src/dtx/dtx_internal.h b/src/dtx/dtx_internal.h index 6a7442c69..0250db7b9 100644 --- a/src/dtx/dtx_internal.h +++ b/src/dtx/dtx_internal.h @@ -77,16 +77,11 @@ CRT_RPC_DECLARE(dtx, DAOS_ISEQ_DTX, DAOS_OSEQ_DTX);

  • dci_hints is sparse array, one per engine, sorted against the rank ID.

  • It can hold more than 19K engines inline RPC body.
    */
    -#define DAOS_ISEQ_COLL_DTX \

  • ((uuid_t) (dci_po_uuid) CRT_VAR) \

  • ((uuid_t) (dci_co_uuid) CRT_VAR) \

  • ((struct dtx_id) (dci_xid) CRT_VAR) \

  • ((uint32_t) (dci_version) CRT_VAR) \

  • ((uint32_t) (dci_min_rank) CRT_VAR) \

  • ((uint32_t) (dci_max_rank) CRT_VAR) \

  • ((uint32_t) (dci_padding) CRT_VAR) \

  • ((uint64_t) (dci_epoch) CRT_VAR) \

  • ((uint8_t) (dci_hints) CRT_ARRAY)
    +#define DAOS_ISEQ_COLL_DTX \

  • ((uuid_t)(dci_po_uuid)CRT_VAR)((uuid_t)(dci_co_uuid)CRT_VAR)((struct dtx_id)( \

  •   dci_xid)CRT_VAR)((uint32_t)(dci_version)CRT_VAR)((uint32_t)(dci_min_rank)CRT_VAR)(     \

  •   (uint32_t)(dci_max_rank)CRT_VAR)((uint32_t)(dci_padding)CRT_VAR)(                      \

  •   (uint64_t)(dci_epoch)CRT_VAR)((uint8_t)(dci_hints)CRT_ARRAY)

@daltonbohning , Honestly, I do not think that the suggested clang-format is friendly for both engineers and other readers. I am wondering why we have to follow such format? or is there any rule can be adjusted? Thanks!

For cases where it sucks, you can add comments

/* clang-format off /
Your code section where it ends up ugly
/ clang-format on */

@jolivier23
Copy link
Contributor

@daltonbohning , Honestly, I do not think that the suggested clang-format is friendly for both engineers and other readers. I am wondering why we have to follow such format? or is there any rule can be adjusted? Thanks!

The hook will format for you https://github.com/daos-stack/daos/blob/master/utils/githooks/README.md

clang-format actually does a very good job of maintaining consistency in most cases but you do have the option to disable it in certain sections of code.

Installing the githooks will save you a lot of hassle otherwise.

@jolivier23
Copy link
Contributor

to answer the other question, the format is controlled by .clang-format file in the daos root tree

@Nasf-Fan Nasf-Fan force-pushed the Nasf-Fan/DAOS-16936_1 branch from 15e44d6 to b279dd6 Compare February 28, 2025 03:08
@Nasf-Fan
Copy link
Contributor Author

Thanks @daltonbohning @jolivier23

@Nasf-Fan
DAOS-16936 dtx: rank range based DTX hints for coll_punch
20c76c8 
When handle collective punch RPC, the leader will generate hints
array for related DTX RPC (abort or commit). Each involved engine
will have one element in such array. To simplify RPC logic, such
hints array is sparse and indexed with engine's rank#. Originally,
we did not properly handled the case of incontinuous rank# in the
pool map, as to when update some hints element with large rank#,
the write maybe out of boundary and crash the others' space and
cause kinds of DRAM corruption.

Similar situation can happen during handle collective DTX resync
and cleanup.

This patch fixes such issue via building such sparse hints array
based on related engines' rank# range instead of the ranks count
in the pool map. Use relative ranks diff (real rank# - base rank#)
as the index. That will avoid out of boundary access.

Signed-off-by: Fan Yong <[email protected]>
@Nasf-Fan Nasf-Fan force-pushed the Nasf-Fan/DAOS-16936_1 branch from b279dd6 to 20c76c8 Compare February 28, 2025 03:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mchaarawi mchaarawi mchaarawi left review comments

@daltonbohning daltonbohning Awaiting requested review from daltonbohning

@liuxuezhao liuxuezhao Awaiting requested review from liuxuezhao

@gnailzenh gnailzenh Awaiting requested review from gnailzenh

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Nasf-Fan @daosbuild1 @daltonbohning @jolivier23 @mchaarawi